cargo-fmt: remove command line validation which did not properly handle the windows environment #3309

scampi · 2019-02-01T00:10:15Z

@jakoschiko @chris-morgan Could you give this branch a try ?

Maybe the problem in windows comes from our handling of env::args(), where the first item is skipped. That is usually the executable name, but not always:

The first element is traditionally the path of the executable, but it can be set to arbitrary text, and may not even exist. This means this property should not be relied upon for security purposes.

Close #2694

chris-morgan · 2019-02-01T00:15:28Z

Haven’t tested it yet, but have skimmed the diff, and I’m >98% sure this won’t fix the problem, as it looks to only affect which arguments are slurped, which is not where the problem is (e.g. “package `member` is not a member of the workspace” indicates that it successfully slurped the package name, but that its workspace member list was awry).

scampi · 2019-02-01T00:21:40Z

@chris-morgan on windows I suspect it's because we call skip(1), I've opened #3310 to track this.
That fix should avoid getting the Invalid argument: member. error when passing --package member or -p member.

scampi · 2019-02-01T00:23:15Z

Actually could you try removing skip(1) and see what happens on your machine ?

jakoschiko · 2019-02-01T23:22:05Z

I'm using the setup from #2694 on Windows 10.

On branch scampi:issue2694:

$ echo $PWD
/d/code/rust/testfmt/member
$ cargo-fmt --version
rustfmt 1.0.1-nightly (2a76d955 2019-02-01)
$ cargo-fmt
Failed to find targets
usage: cargo fmt [options]

On branch scampi:issue2694 and with skip(1) removed:

$ cargo-fmt
Invalid argument: `C:\Users\anonymous\.cargo\bin\cargo-fmt.exe`.
usage: cargo fmt [options]

jakoschiko · 2019-02-01T23:51:39Z

I added some dbgs to get_targets_root_only:

fn get_targets_root_only(targets: &mut HashSet<Target>) -> Result<(), io::Error> {
    let metadata = get_cargo_metadata(None)?;
    let current_dir = dbg!(dbg!(env::current_dir()?).canonicalize()?);
    let current_dir_manifest = dbg!(current_dir.join("Cargo.toml"));
    let workspace_root_path = dbg!(PathBuf::from(&metadata.workspace_root).canonicalize()?);
    let in_workspace_root = dbg!(workspace_root_path == current_dir);

    for package in metadata.packages {
        if in_workspace_root || dbg!(dbg!(PathBuf::from(&package.manifest_path)) == current_dir_manifest) {
            for target in package.targets {
                targets.insert(Target::from_target(&target));
            }
        }
    }

    Ok(())
}

Here the result:

$ cargo-fmt
[src/cargo-fmt/main.rs:239] env::current_dir()? = "D:\\Code\\rust\\testfmt\\member"
[src/cargo-fmt/main.rs:239] dbg!(env :: current_dir (  ) ?).canonicalize()? = "\\\\?\\D:\\Code\\rust\\testfmt\\member"
[src/cargo-fmt/main.rs:240] current_dir.join("Cargo.toml") = "\\\\?\\D:\\Code\\rust\\testfmt\\member\\Cargo.toml"
[src/cargo-fmt/main.rs:241] PathBuf::from(&metadata.workspace_root).canonicalize()? = "\\\\?\\D:\\Code\\rust\\testfmt"
[src/cargo-fmt/main.rs:242] workspace_root_path == current_dir = false
[src/cargo-fmt/main.rs:245] PathBuf::from(&package.manifest_path) = "D:\\Code\\rust\\testfmt\\member\\Cargo.toml"
[src/cargo-fmt/main.rs:245] dbg!(PathBuf :: from ( & package . manifest_path )) == current_dir_manifest = false
Failed to find targets
usage: cargo fmt [options]

The result of canonicalize looks strange.

Edit: Changing PathBuf::from(&package.manifest_path) to PathBuf::from(&package.manifest_path).canonicalize()? fixes the problem.

scampi · 2019-02-02T17:32:33Z

Thanks @jakoschiko for the help.

You're right, canonicalize on Windows returns an extended-length path which adds the leading "\\?\" we see in your debug. I'll apply your fix for this.

Regarding skip(1) it would be kinda OK if the following were removed; I was just trying to figure out what could explain the various errors reported in the issue, and anyway that skipping will be further worked on in a separate PR (issue #3310).

rustfmt/src/cargo-fmt/main.rs

Lines 55 to 66 in 2a76d95

    
           // If there is any invalid argument passed to `cargo fmt`, return without formatting. 
        
           let mut is_package_arg = false; 
        
           for arg in env::args().skip(1).take_while(|a| a != "--") { 
        
               if arg.starts_with('-') { 
        
                   is_package_arg = arg.starts_with("--package") | arg.starts_with("-p"); 
        
               } else if !is_package_arg { 
        
                   print_usage_to_stderr(&opts, &format!("Invalid argument: `{}`.", arg)); 
        
                   return FAILURE; 
        
               } else { 
        
                   is_package_arg = false; 
        
               } 
        
           }

scampi · 2019-02-02T17:36:06Z

The following error was also reported

C:\workspace\member>cargo fmt --package member
package `member` is not a member of the workspace
[usage]

@jakoschiko Could you also provide a debug of the following line ? thanks

rustfmt/src/cargo-fmt/main.rs

Line 296 in 2a76d95

if workspace_hitlist.remove(&package.name) {

@jakoschiko

…leading "\\?\" which needs to be taken into account apply @jakoschiko fix

jakoschiko · 2019-02-02T21:14:55Z

Using stable:

$ echo $PWD
/d/code/rust/testfmt
$ cargo fmt --version
rustfmt 1.0.0-stable (43206f41 2018-11-30)
$ cargo fmt --package member
$ cargo fmt -p member
Invalid argument: `member`.
usage: cargo fmt [options]
...
$ cargo fmt -pmember
$ cd member
$ cargo fmt --package member
$ cargo fmt -p member
Invalid argument: `member`.
usage: cargo fmt [options]
...
$ cargo fmt -pmember

Using your current branch with the dbgs:

$ echo $PWD
/d/code/rust/testfmt
$ cargo fmt --version
Invalid argument: `fmt`.
usage: cargo fmt [options]
...
$ cargo-fmt --version
rustfmt 1.0.1-nightly (06d8feb8 2019-02-02)
$ cargo-fmt --package member
[src/cargo-fmt/main.rs:298] &package.name = "member"
[src/cargo-fmt/main.rs:298] workspace_hitlist.remove(dbg!(& package . name)) = true
$ cargo-fmt -p member
[src/cargo-fmt/main.rs:298] &package.name = "member"
[src/cargo-fmt/main.rs:298] workspace_hitlist.remove(dbg!(& package . name)) = true
$ cargo-fmt -pmember
[src/cargo-fmt/main.rs:298] &package.name = "member"
[src/cargo-fmt/main.rs:298] workspace_hitlist.remove(dbg!(& package . name)) = true
$ cd member
$ cargo-fmt --package member
[src/cargo-fmt/main.rs:298] &package.name = "member"
[src/cargo-fmt/main.rs:298] workspace_hitlist.remove(dbg!(& package . name)) = true
$ cargo-fmt -p member
[src/cargo-fmt/main.rs:298] &package.name = "member"
[src/cargo-fmt/main.rs:298] workspace_hitlist.remove(dbg!(& package . name)) = true
$ cargo-fmt -pmember
[src/cargo-fmt/main.rs:298] &package.name = "member"
[src/cargo-fmt/main.rs:298] workspace_hitlist.remove(dbg!(& package . name)) = true

-p member seems to work. But because of skip(1) I can't use rustfmt as cargo subcommand. With skip(2):

$ echo $PWD
/d/code/rust/testfmt
$ cargo fmt --version
rustfmt 1.0.1-nightly (06d8feb8 2019-02-02)
$ cargo fmt --package member
[src/cargo-fmt/main.rs:298] &package.name = "member"
[src/cargo-fmt/main.rs:298] workspace_hitlist.remove(dbg!(& package . name)) = true
$ cargo fmt -p member
[src/cargo-fmt/main.rs:298] &package.name = "member"
[src/cargo-fmt/main.rs:298] workspace_hitlist.remove(dbg!(& package . name)) = true
$ cargo fmt -pmember
[src/cargo-fmt/main.rs:298] &package.name = "member"
[src/cargo-fmt/main.rs:298] workspace_hitlist.remove(dbg!(& package . name)) = true
$ cd member
$ cargo fmt --package member
[src/cargo-fmt/main.rs:298] &package.name = "member"
[src/cargo-fmt/main.rs:298] workspace_hitlist.remove(dbg!(& package . name)) = true
$ cargo fmt -p member
[src/cargo-fmt/main.rs:298] &package.name = "member"
[src/cargo-fmt/main.rs:298] workspace_hitlist.remove(dbg!(& package . name)) = true
$ cargo fmt -pmember
[src/cargo-fmt/main.rs:298] &package.name = "member"
[src/cargo-fmt/main.rs:298] workspace_hitlist.remove(dbg!(& package . name)) = true

…when called with 'cargo fmt'

scampi · 2019-02-05T20:07:42Z

@jakoschiko I reverted the change to skip, as I didn't test this properly before proposing the PR.
I've checked the branch as of ec8ec05 and it seems to work fine to me.

I tried the changes locally and used the following debugs in the gif below. I didn't manage to reproduce @chris-morgan 's error:

C:\workspace\member>cargo fmt --package member
package `member` is not a member of the workspace
[usage]

I think this PR is fine now.

@@ -54,7 +54,7 @@ fn execute() -> i32 {
 
     // If there is any invalid argument passed to `cargo fmt`, return without formatting.
     let mut is_package_arg = false;
-    for arg in env::args().skip(1).take_while(|a| a != "--") {
+    for arg in dbg!(env::args()).skip(2).take_while(|a| a != "--") {
         if arg.starts_with('-') {
             is_package_arg = arg.starts_with("--package") | arg.starts_with("-p");
         } else if !is_package_arg {
@@ -224,7 +224,7 @@ fn get_targets(strategy: &CargoFmtStrategy) -> Result<HashSet<Target>, io::Error
         CargoFmtStrategy::Some(ref hitlist) => get_targets_with_hitlist(hitlist, &mut targets)?,
     }
 
-    if targets.is_empty() {
+    if dbg!(&targets).is_empty() {
         Err(io::Error::new(
             io::ErrorKind::Other,
             "Failed to find targets".to_owned(),
@@ -292,10 +292,10 @@ fn get_targets_with_hitlist(
 ) -> Result<(), io::Error> {
     let metadata = get_cargo_metadata(None)?;
 
-    let mut workspace_hitlist: HashSet<&String> = HashSet::from_iter(hitlist);
+    let mut workspace_hitlist: HashSet<&String> = HashSet::from_iter(dbg!(hitlist));
 
     for package in metadata.packages {
-        if workspace_hitlist.remove(&package.name) {
+        if workspace_hitlist.remove(dbg!(&package.name)) {
             for target in package.targets {
                 targets.insert(Target::from_target(&target));
             }

topecongiro · 2019-10-19T07:49:27Z

I am closing this in favor of #3652. @scampi Thank you for your work, and sorry that the review got delayed. For better or worse, I now have a Windows machine at my hand, so testing this kind of error should be more straightforward.

cargo-fmt: skip only the executable name

2a76d95

canonicalize on Windows returns an extended-length path which adds a …

06d8feb

…leading "\\?\" which needs to be taken into account apply @jakoschiko fix

cargo-fmt: skip the first two args which are the bin's name and fmt, …

ec8ec05

…when called with 'cargo fmt'

scampi mentioned this pull request Mar 31, 2019

[WIP] Use absolute_path instead of fs::canonicalize #3324

Closed

jakoschiko mentioned this pull request May 23, 2019

cargo-fmt in (virtual) workspace member directories fails #2694

Closed

calebcartwright mentioned this pull request Jun 25, 2019

fix: support running in sub directories/workspace member directories #3652

Merged

topecongiro closed this Oct 19, 2019

scampi deleted the issue2694 branch October 21, 2019 07:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cargo-fmt: remove command line validation which did not properly handle the windows environment #3309

cargo-fmt: remove command line validation which did not properly handle the windows environment #3309

scampi commented Feb 1, 2019

chris-morgan commented Feb 1, 2019

scampi commented Feb 1, 2019

scampi commented Feb 1, 2019

jakoschiko commented Feb 1, 2019

jakoschiko commented Feb 1, 2019 •

edited

Loading

scampi commented Feb 2, 2019

scampi commented Feb 2, 2019

jakoschiko commented Feb 2, 2019

scampi commented Feb 5, 2019

topecongiro commented Oct 19, 2019

cargo-fmt: remove command line validation which did not properly handle the windows environment #3309

cargo-fmt: remove command line validation which did not properly handle the windows environment #3309

Conversation

scampi commented Feb 1, 2019

chris-morgan commented Feb 1, 2019

scampi commented Feb 1, 2019

scampi commented Feb 1, 2019

jakoschiko commented Feb 1, 2019

jakoschiko commented Feb 1, 2019 • edited Loading

scampi commented Feb 2, 2019

scampi commented Feb 2, 2019

jakoschiko commented Feb 2, 2019

scampi commented Feb 5, 2019

topecongiro commented Oct 19, 2019

jakoschiko commented Feb 1, 2019 •

edited

Loading